Skip to content

refactor(deps): Document standard rules for writing lib install tasks and apply across all existing tasks: #1479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

refactor(deps): Document standard rules for writing lib install tasks and apply across all existing tasks: #1479

wants to merge 21 commits into from
+112 −257

Conversation

@Bill-hbrhbr
Copy link
Contributor

@Bill-hbrhbr Bill-hbrhbr commented Oct 23, 2025 •
edited
Loading

  • Modify CMAKE_GEN_ARGS of each task to align with the newly established conventions.
  • Ensure all CLP targets link against static export targets.
  • Remove redundant checksum validation from antlr-jar.
  • Refactor antlr-runtime to build using utils:install-remote-cmake-lib.
  • Make spider platform check POSIX-compatible.
  • Replace multi-line echo with heredoc (cat <<EOF) for clearer text generation.

Description

Due to various CMAKE_GEN_ARGS issues encountered while reviewing other PRs such as #1377 and #1414, we standardize CMake lib writing once and for all to cut down on future review/development time.

One important feature is that we enforce using static library artifacts and export targets whenever possible when linking against CLP targets.

Running:

find . -type f -name '*.so.*' -print

under clp/build/deps/cpp returns:

./boost-build/boost/bin.v2/libs/filesystem/config/gcc-11/release/x86_64/threading-multi/visibility-hidden/libhas_linkflag_no_undefined.so.1.87.0
./boost-build/boost/bin.v2/libs/filesystem/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_filesystem.so.1.87.0
./boost-build/boost/bin.v2/libs/context/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_context.so.1.87.0
./boost-build/boost/bin.v2/libs/random/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_random.so.1.87.0
./boost-build/boost/bin.v2/libs/program_options/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_program_options.so.1.87.0
./boost-build/boost/bin.v2/libs/system/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_system.so.1.87.0
./boost-build/boost/bin.v2/libs/process/build/gcc-11/release/x86_64/boost.process.fs-boost/threading-multi/visibility-hidden/libboost_process.so.1.87.0
./boost-build/boost/bin.v2/libs/url/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_url.so.1.87.0
./boost-build/boost/bin.v2/libs/atomic/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_atomic.so.1.87.0
./boost-build/boost/bin.v2/libs/date_time/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_date_time.so.1.87.0
./boost-build/boost/bin.v2/libs/iostreams/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_iostreams.so.1.87.0
./boost-build/boost/bin.v2/libs/regex/build/gcc-11/release/x86_64/threading-multi/visibility-hidden/libboost_regex.so.1.87.0
./zlib-build/libz.so.1.3.1
./mongocxx-build/src/bsoncxx/libbsoncxx.so.4.1.1
./mongocxx-build/src/mongocxx/libmongocxx.so.4.1.1
./mongocxx-build/_deps/mongo-c-driver-build/src/libbson/libbson2.so.2.0.0
./mongocxx-build/_deps/mongo-c-driver-build/src/libmongoc/libmongoc2.so.2.0.0
./mongocxx-install/lib/libmongocxx.so.4.1.1
./mongocxx-install/lib/libbsoncxx.so.4.1.1
./mongocxx-install/lib/libmongoc2.so.2.0.0
./mongocxx-install/lib/libbson2.so.2.0.0
./zlib-install/lib/libz.so.1.3.1
./boost-install/lib/libboost_process.so.1.87.0
./boost-install/lib/libboost_system.so.1.87.0
./boost-install/lib/libboost_atomic.so.1.87.0
./boost-install/lib/libboost_url.so.1.87.0
./boost-install/lib/libboost_program_options.so.1.87.0
./boost-install/lib/libboost_iostreams.so.1.87.0
./boost-install/lib/libboost_filesystem.so.1.87.0
./boost-install/lib/libboost_context.so.1.87.0
./boost-install/lib/libboost_date_time.so.1.87.0
./boost-install/lib/libboost_regex.so.1.87.0
./boost-install/lib/libboost_random.so.1.87.0
  • Boost is a special cases handled by yscope-dev-utils
  • There is no way to disable shared artifacts building for mongocxx and zlib
    • For mongocxx, we link against the static export target mongo::mongocxx_static
    • We do not call find_package() for zlib. It will be used by LibArchive when we switch it to task-based installation.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Dep tasks run successfully and help build functionally correct binaries.

Summary by CodeRabbit

  • Documentation

    • Added a Dependency Taskfiles README with guidelines for dependency installation, preferred installer arguments, checksum guidance and tarball extraction naming.

  • Chores

    • Centralized and simplified dependency installation flows with standardized installer arguments and unified build defaults (Release, skip tests/docs by default); many components moved to fixed-version tarballs.

  • Compatibility

    • Simplified dependency discovery/installation behavior for certain libraries and compression integrations; expect more consistent, standardized installs.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025 •
edited
Loading

Walkthrough

Adds dependency Taskfile documentation and refactors taskfiles/deps/main.yaml to centralize remote CMake installs via deps:utils:install-remote-cmake-lib, inlines tarball URLs and SHA256s, normalizes CMake generator arguments, standardizes extraction directory naming (-extracted), removes many per-package download/build/checksum steps, and deletes the custom LibLZMA finder while using find_package(LibLZMA 5.8.1 REQUIRED) in components/core/CMakeLists.txt.

Changes

Cohort / File(s) Summary
Documentation
taskfiles/deps/README.md		 New guide for dependency Taskfiles: preferred installer tasks, checksum guidance, extraction directory naming, avoidance of URL parsing, and explicit CMAKE_GEN_ARGS rules (CMP0074, prefer static/shared flags, disable tests/docs/examples, default CMAKE_BUILD_TYPE=Release, CMAKE_INSTALL_MESSAGE=LAZY).
Centralized installer & orchestration
taskfiles/deps/main.yaml		 Major refactor: many entries now call deps:utils:install-remote-cmake-lib with standardized params (LIB_NAME, TARBALL_URL, TARBALL_SHA256, CMAKE_GEN_ARGS, CMAKE_SOURCE_DIR); precondition sourcing simplified; removed per-package checksum/download/build steps; extraction naming standardized to -extracted; public variable signatures adjusted.
Per-package consolidations
taskfiles/deps/main.yaml — antlr-jar, antlr-runtime, boost, catch2, liblzma, mariadb-connector-cpp, mongocxx, msgpack-cxx, simdjson, spdlog, sqlite3, utfcpp, yaml-cpp, ystdlib, zstd, ...		 Removed many legacy public vars (e.g., VERSION, CHECKSUM_FILE, OUTPUT_FILE); inlined fixed TARBALL_URL/TARBALL_SHA256 where available; moved build flags into CMAKE_GEN_ARGS; exposed new flags (e.g., shared/static, BUILD_TYPE, INSTALL_MESSAGE) where needed; changed extraction dir naming and simplified generation blocks.
CMake core adjustments
components/core/CMakeLists.txt		 Removed conditional static-vs-shared selection and LibLZMA root/use-static logic; now uses find_package(LibLZMA 5.8.1 REQUIRED) and assigns fixed targets for mongocxx/zstd.
Removed CMake module
components/core/cmake/Modules/FindLibLZMA.cmake		 Deleted custom LibLZMA discovery module (pkg-config handling, header/library locating, optional static preference, imported target creation, and exported LibLZMA_* variables/target removed).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Developer
  participant Runner as Taskfile runner
  participant Installer as deps:utils:install-remote-cmake-lib
  participant Remote as Tarball host
  participant Build as CMake / Build system
  Note over Runner,Installer `#D6EAF8`: Centralized install flow per dependency
  Developer->>Runner: invoke dependency task
  Runner->>Installer: call(LIB_NAME, TARBALL_URL, TARBALL_SHA256, CMAKE_GEN_ARGS, CMAKE_SOURCE_DIR)
  Installer->>Remote: download tarball (TARBALL_URL)
  alt SHA256 provided
    Remote-->>Installer: tarball
    Installer->>Installer: verify SHA256
  else no SHA256
    Remote-->>Installer: tarball
  end
  Installer->>Installer: extract to <lib>-extracted
  Installer->>Build: cmake configure (CMAKE_GEN_ARGS)
  Build->>Build: build & install per args (shared/static, tests off/on)
  Build-->>Installer: artifacts installed
  Installer-->>Runner: return status
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Pay special attention to:
    • Removal of FindLibLZMA.cmake and its exported variables/IMPORTED target and the impact on platforms, static vs shared selection, and consumers expecting LibLZMA_* variables or LibLZMA::LibLZMA.
    • Widespread public-variable signature changes in taskfiles/deps/main.yaml and downstream consumers relying on removed names (e.g., VERSION, CHECKSUM_FILE, OUTPUT_FILE).
    • Correctness of inlined TARBALL_URL and TARBALL_SHA256 values and any missing checksum usages.
    • Behavioural changes from standardized CMAKE_GEN_ARGS (CMP0074, disabling tests/examples/docs, shared/static flags).
    • Repository references to renamed extraction directories (e.g., -extracted, UTFCPP_OUTPUT_DIR).

Possibly related issues

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and specifically describes the main changes: documenting standard rules for dependency installation tasks and applying them across all existing tasks.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Bill-hbrhbr
Copy link
Contributor Author

Bill-hbrhbr commented Oct 23, 2025 •
edited
Loading

@coderabbitai

give me a diff of each lib install task in the current version against the files in PR #1377, diff should include comments as well.

@Bill-hbrhbr
Copy link
Contributor Author

Bill-hbrhbr commented Oct 23, 2025

@coderabbitai Diff should include the comments inside the tasks. not your own comments.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# First, let's see what lib install tasks are in the current PR
echo "=== Lib install task files in current PR ==="
gh pr view 1479 --json files --jq '.files[].path' | grep -E 'lib.*install.*\.ya?ml'

echo ""
echo "=== Looking for task files more broadly ==="
gh pr view 1479 --json files --jq '.files[].path' | grep -E '\.ya?ml$'

Length of output: 114

🏁 Script executed:

#!/bin/bash

# Let's also check the directory structure to understand where lib install tasks are
echo "=== Finding YAML files in components/core/tools/scripts/lib_install ==="
fd -e yml -e yaml . components/core/tools/scripts/lib_install 2>/dev/null || echo "Directory not found, searching more broadly..."

echo ""
echo "=== All YAML task files in the repo ==="
fd -e yml -e yaml . components/core/tools/scripts 2>/dev/null | head -20

Length of output: 278

🏁 Script executed:

#!/bin/bash

# Get the list of changed files in PR #1479
echo "=== All changed files in PR #1479 ==="
gh pr view 1479 --json files --jq '.files[] | .path'

Length of output: 62

🏁 Script executed:

#!/bin/bash

# Let's see what files were changed in PR #1377
echo "=== Files changed in PR #1377 ==="
gh pr view 1377 --json files --jq '.files[] | .path' | sort

echo ""
echo "=== Looking for lib install or taskfile changes in PR #1377 ==="
gh pr view 1377 --json files --jq '.files[] | .path' | grep -E '(taskfile|lib_install|deps)'

Length of output: 2174

🏁 Script executed:

#!/bin/bash

# Let's get the content of taskfiles/deps/main.yaml from the current PR #1479
echo "=== Current version in PR #1479 ==="
cat taskfiles/deps/main.yaml

Length of output: 20857

🏁 Script executed:

#!/bin/bash

# Let's fetch the version from PR #1377
# First, get the branch name or commit from PR #1377
echo "=== PR #1377 details ==="
gh pr view 1377 --json headRefName,number,title

echo ""
echo "=== Fetching taskfiles/deps/main.yaml from PR #1377 ==="
# Get the head ref of PR #1377
HEAD_REF=$(gh pr view 1377 --json headRefName --jq '.headRefName')
echo "Head ref: $HEAD_REF"

# Fetch the file from that branch
git fetch origin "$HEAD_REF:pr-1377-branch" 2>/dev/null || echo "Could not fetch branch"
git show "pr-1377-branch:taskfiles/deps/main.yaml" 2>/dev/null || echo "File not found in PR #1377"

Length of output: 388

🏁 Script executed:

#!/bin/bash

# Let's try fetching the file content directly using gh api
echo "=== Getting file content from PR #1377 ==="
gh api repos/y-scope/clp/pulls/1377/files --jq '.[] | select(.filename == "taskfiles/deps/main.yaml") | .raw_url' | head -1 | xargs -I {} curl -s {}

Length of output: 42

🏁 Script executed:

#!/bin/bash

# Now let's get all the install task files from PR #1377
echo "=== Getting all install task files from PR #1377 ==="
gh api repos/y-scope/clp/pulls/1377/files --jq '.[] | select(.filename | contains("taskfiles/deps/installs/")) | .filename'

Length of output: 956

🏁 Script executed:

#!/bin/bash

# Get the content of all install task files from current PR
echo "=== Checking if install tasks directory exists in current PR ==="
ls -la taskfiles/deps/installs/ 2>/dev/null || echo "Directory does not exist in current PR"

echo ""
echo "=== Checking directory structure ==="
find taskfiles/deps -type f -name "*.yaml" 2>/dev/null

Length of output: 217

🏁 Script executed:

#!/bin/bash

# Get raw URLs for all install task files from PR #1377
echo "=== Fetching raw URLs for install tasks from PR #1377 ==="
gh api repos/y-scope/clp/pulls/1377/files --jq '.[] | select(.filename | contains("taskfiles/deps/installs/")) | "\(.filename) \(.raw_url)"'

Length of output: 3833

🏁 Script executed:

#!/bin/bash

# Extract individual lib install tasks from current main.yaml and create temp files for diffing
# Let's use awk to extract each task

# List of lib install tasks to extract
LIBS="absl antlr-jar antlr-runtime boost catch2 date fmt liblzma liblzma-install log-surgeon lz4 mariadb-connector-cpp microsoft.gsl mongocxx msgpack-cxx nlohmann_json simdjson spdlog sqlite3 utfcpp yaml-cpp ystdlib zlib zstd"

mkdir -p /tmp/current-tasks

for lib in $LIBS; do
  echo "Extracting task: $lib"
  awk -v lib="$lib" '
    BEGIN { in_task = 0; indent = "" }
    /^[[:space:]]*'"$lib"':/ && !/'"$lib"'-/ { 
      in_task = 1
      print
      # Get the indent level
      match($0, /^[[:space:]]*/)
      indent = substr($0, RSTART, RLENGTH)
      next
    }
    in_task {
      # Check if we hit another task at the same level
      if (/^[[:space:]]*[a-zA-Z0-9_-]+:/ && match($0, /^[[:space:]]*/)) {
        current_indent = substr($0, RSTART, RLENGTH)
        if (length(current_indent) <= length(indent)) {
          exit
        }
      }
      print
    }
  ' taskfiles/deps/main.yaml > /tmp/current-tasks/${lib}.yaml
done

echo "Tasks extracted to /tmp/current-tasks/"
ls -la /tmp/current-tasks/

Length of output: 2294

🏁 Script executed:

#!/bin/bash

# Download all install task files from PR #1377
mkdir -p /tmp/pr-1377-tasks

echo "=== Downloading install task files from PR #1377 ==="

# Download each file
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fabsl.yaml" > /tmp/pr-1377-tasks/absl.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fantlr-jar.yaml" > /tmp/pr-1377-tasks/antlr-jar.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fantlr-runtime.yaml" > /tmp/pr-1377-tasks/antlr-runtime.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fboost.yaml" > /tmp/pr-1377-tasks/boost.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fcatch2.yaml" > /tmp/pr-1377-tasks/catch2.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fdate.yaml" > /tmp/pr-1377-tasks/date.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Ffmt.yaml" > /tmp/pr-1377-tasks/fmt.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fliblzma.yaml" > /tmp/pr-1377-tasks/liblzma.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Flog-surgeon.yaml" > /tmp/pr-1377-tasks/log-surgeon.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Flz4.yaml" > /tmp/pr-1377-tasks/lz4.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fmariadb-connector-cpp.yaml" > /tmp/pr-1377-tasks/mariadb-connector-cpp.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fmicrosoft.gsl.yaml" > /tmp/pr-1377-tasks/microsoft.gsl.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fmongocxx.yaml" > /tmp/pr-1377-tasks/mongocxx.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fmsgpack-cxx.yaml" > /tmp/pr-1377-tasks/msgpack-cxx.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fnlohmann_json.yaml" > /tmp/pr-1377-tasks/nlohmann_json.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fsimdjson.yaml" > /tmp/pr-1377-tasks/simdjson.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fspdlog.yaml" > /tmp/pr-1377-tasks/spdlog.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fsqlite3.yaml" > /tmp/pr-1377-tasks/sqlite3.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Futfcpp.yaml" > /tmp/pr-1377-tasks/utfcpp.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fyaml-cpp.yaml" > /tmp/pr-1377-tasks/yaml-cpp.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fystdlib.yaml" > /tmp/pr-1377-tasks/ystdlib.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fzlib.yaml" > /tmp/pr-1377-tasks/zlib.yaml
curl -s "https://github.com/y-scope/clp/raw/a143bda8fd871a47d05af81b545319c0dc8fc2b9/taskfiles%2Fdeps%2Finstalls%2Fzstd.yaml" > /tmp/pr-1377-tasks/zstd.yaml

echo "Files downloaded"
ls -la /tmp/pr-1377-tasks/

Length of output: 1601

🏁 Script executed:

#!/bin/bash

# Now generate diffs for each lib install task
echo "=== Generating diffs for each lib install task ==="
echo ""

LIBS="absl antlr-jar antlr-runtime boost catch2 date fmt liblzma log-surgeon lz4 mariadb-connector-cpp microsoft.gsl mongocxx msgpack-cxx nlohmann_json simdjson spdlog sqlite3 utfcpp yaml-cpp ystdlib zlib zstd"

for lib in $LIBS; do
  echo "=========================================="
  echo "DIFF for: $lib"
  echo "=========================================="
  
  if [ -f "/tmp/pr-1377-tasks/${lib}.yaml" ] && [ -s "/tmp/current-tasks/${lib}.yaml" ]; then
    diff -u "/tmp/pr-1377-tasks/${lib}.yaml" "/tmp/current-tasks/${lib}.yaml" || true
  else
    if [ ! -f "/tmp/pr-1377-tasks/${lib}.yaml" ]; then
      echo "File not found in PR #1377"
    fi
    if [ ! -s "/tmp/current-tasks/${lib}.yaml" ]; then
      echo "File empty or not found in current PR"
    fi
  fi
  echo ""
  echo ""
done

Length of output: 20921

🏁 Script executed:

#!/bin/bash

# The URLs were not working, let's try using gh api to get the raw content
echo "=== Fetching content from PR #1377 using gh api ==="

# Let's try to get the files using gh api with proper decoding
mkdir -p /tmp/pr-1377-tasks-v2

# List of libraries
LIBS="absl antlr-jar antlr-runtime boost catch2 date fmt liblzma log-surgeon lz4 mariadb-connector-cpp microsoft.gsl mongocxx msgpack-cxx nlohmann_json simdjson spdlog sqlite3 utfcpp yaml-cpp ystdlib zlib zstd"

# Get the commit SHA for PR #1377
COMMIT_SHA="a143bda8fd871a47d05af81b545319c0dc8fc2b9"

for lib in $LIBS; do
  echo "Fetching $lib.yaml..."
  gh api -H "Accept: application/vnd.github.v3.raw" \
    "/repos/y-scope/clp/contents/taskfiles/deps/installs/${lib}.yaml?ref=${COMMIT_SHA}" \
    > /tmp/pr-1377-tasks-v2/${lib}.yaml 2>/dev/null || echo "Failed to fetch ${lib}.yaml"
done

echo ""
echo "Files downloaded:"
ls -lh /tmp/pr-1377-tasks-v2/

Length of output: 2121

🏁 Script executed:

#!/bin/bash

# Now generate clean diffs for each lib install task
echo "Generating diffs between PR #1377 and current PR #1479"
echo "Format: --- PR #1377 version, +++ Current PR #1479 version"
echo ""

LIBS="absl antlr-jar antlr-runtime boost catch2 date fmt liblzma log-surgeon lz4 mariadb-connector-cpp microsoft.gsl mongocxx msgpack-cxx nlohmann_json simdjson spdlog sqlite3 utfcpp yaml-cpp ystdlib zlib zstd"

for lib in $LIBS; do
  echo "=========================================="
  echo "DIFF: $lib"
  echo "=========================================="
  
  if [ -f "/tmp/pr-1377-tasks-v2/${lib}.yaml" ] && [ -s "/tmp/current-tasks/${lib}.yaml" ]; then
    diff -u "/tmp/pr-1377-tasks-v2/${lib}.yaml" "/tmp/current-tasks/${lib}.yaml" || true
  else
    if [ ! -f "/tmp/pr-1377-tasks-v2/${lib}.yaml" ]; then
      echo "ERROR: File not found in PR #1377"
    fi
    if [ ! -s "/tmp/current-tasks/${lib}.yaml" ]; then
      echo "ERROR: File empty or not found in current PR"
    fi
  fi
  echo ""
done

echo "=========================================="
echo "SPECIAL CASE: liblzma-install"
echo "=========================================="
echo "NOTE: liblzma-install is an internal task in current PR but not a separate file in PR #1377"
cat /tmp/current-tasks/liblzma-install.yaml
echo ""

Length of output: 23007

@Bill-hbrhbr: Here are the diffs for each lib install task between PR #1377 and current PR #1479, with all comments inside the tasks preserved:

The key structural differences across all tasks:

PR #1377 structure (separate files):

version: "3"

tasks:
  default:
    # task content

Current PR #1479 structure (inline in main.yaml):

  task-name:
    # task content

Main changes:

  1. Task references: :task → utils:task or yscope-dev-utils:task
  2. Variables: Template variables like {{.VERSION}} → hardcoded values
  3. Dependencies: for loops → direct task dependencies
  4. Missing CMAKE flags: Some tasks added -DCMAKE_BUILD_TYPE=Release and -DCMAKE_INSTALL_MESSAGE=LAZY (e.g., ystdlib)

Special cases:

All 23 complete lib install tasks have been compared with their full content including comments preserved in the diffs above.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Get the list of files changed in PR #1479 (current PR)
echo "=== Files in PR #1479 ==="
gh pr view 1479 --json files --jq '.files[].path' | sort

echo ""
echo "=== Files in PR #1377 ==="
# Get the list of files changed in PR #1377
gh pr view 1377 --json files --jq '.files[].path' | sort

echo ""
echo "=== PR #1377 details ==="
gh pr view 1377 --json number,title,state,mergedAt,headRefName

Length of output: 1397

🏁 Script executed:

#!/bin/bash

# Get the branch name for PR #1377 to checkout its files
PR_1377_BRANCH=$(gh pr view 1377 --json headRefName --jq '.headRefName')
echo "PR #1377 branch: $PR_1377_BRANCH"

# Fetch the branch
git fetch origin "$PR_1377_BRANCH"

# List lib install task files in the current branch (PR #1479)
echo ""
echo "=== Lib install task files in current PR (1479) ==="
fd -t f -e yml -e yaml . tasks | grep -i 'lib.*install' | sort

echo ""
echo "=== All yml files in tasks directory (current branch) ==="
fd -t f -e yml -e yaml . tasks | sort

Length of output: 390

🏁 Script executed:

#!/bin/bash

# Let's find what files exist in tasks directory on the current branch
echo "=== Current branch task files ==="
find tasks -name "*.yml" -o -name "*.yaml" 2>/dev/null | sort

# Get PR #1377 branch name
PR_1377_BRANCH=$(gh pr view 1377 --json headRefName --jq '.headRefName')

# Checkout PR #1377 branch files to a temporary location
git fetch origin "$PR_1377_BRANCH"

echo ""
echo "=== Task files in PR #1377 branch ==="
git ls-tree -r --name-only "origin/$PR_1377_BRANCH" tasks/ | grep -E '\.(yml|yaml)$' | sort

Length of output: 186

🏁 Script executed:

#!/bin/bash

# Get the exact list of lib install task files from current branch
echo "=== Identifying lib install task files ==="

# Find all yml/yaml files in tasks directory
CURRENT_TASKS=$(find tasks -type f \( -name "*.yml" -o -name "*.yaml" \) 2>/dev/null | sort)

echo "Current task files:"
echo "$CURRENT_TASKS"

# Get PR #1377 branch
PR_1377_BRANCH=$(gh pr view 1377 --json headRefName --jq '.headRefName')
git fetch origin "$PR_1377_BRANCH" 2>/dev/null

echo ""
echo "=== Files from PR #1377 ==="
git ls-tree -r --name-only "origin/$PR_1377_BRANCH" tasks/ 2>/dev/null | grep -E '\.(yml|yaml)$' | sort

Length of output: 92

🏁 Script executed:

#!/bin/bash

# Let's be more specific and get lib install task files
# First, let's see what files are in the tasks directory

echo "=== Current branch lib install tasks ==="
fd -t f 'lib.*install.*\.yml' tasks 2>/dev/null || find tasks -type f -name "*lib*install*.yml" 2>/dev/null

echo ""
echo "=== All current tasks files ==="
find tasks -type f -name "*.yml" | head -20

# Get PR branch
PR_1377_BRANCH=$(gh pr view 1377 --json headRefName --jq '.headRefName')
echo ""
echo "PR #1377 branch: $PR_1377_BRANCH"

# Show files that exist in PR #1377
echo ""
echo "=== Checking out PR #1377 files ==="
git fetch origin "$PR_1377_BRANCH"
git show "origin/$PR_1377_BRANCH":tasks/ 2>/dev/null | head -20 || echo "Directory listing not available"

# List files in tasks from PR #1377
git ls-tree -r --name-only "origin/$PR_1377_BRANCH" -- tasks/ | head -20

Length of output: 305

🏁 Script executed:

#!/bin/bash

# Check the actual directory structure
echo "=== Repository structure ==="
ls -la

echo ""
echo "=== Looking for taskfiles directory ==="
ls -la taskfiles/ 2>/dev/null || echo "taskfiles directory not found"

echo ""
echo "=== Files in taskfiles/deps/ ==="
find taskfiles/deps -type f -name "*.yaml" 2>/dev/null | sort

echo ""
echo "=== Files in taskfiles/deps/installs/ ==="
find taskfiles/deps/installs -type f -name "*.yaml" 2>/dev/null | sort

Length of output: 2128

🏁 Script executed:

#!/bin/bash

# Get file contents from PR #1377 using GitHub API
# First, let's check if we can fetch commits from the PR

echo "=== Getting PR #1377 commit SHA ==="
PR_1377_SHA=$(gh pr view 1377 --json commits --jq '.commits[-1].oid')
echo "Latest commit SHA in PR #1377: $PR_1377_SHA"

# Try fetching that specific commit
git fetch origin "$PR_1377_SHA" 2>&1 || echo "Could not fetch commit"

# List files at that commit
echo ""
echo "=== Files in PR #1377 ==="
git ls-tree -r --name-only "$PR_1377_SHA" -- taskfiles/deps/installs/ 2>/dev/null | sort

Length of output: 1150

🏁 Script executed:

#!/bin/bash

# Alternative approach: Download file contents directly from PR #1377 using GitHub API
# Get the list of install task files from PR #1377

echo "=== Install task files from PR #1377 ==="
INSTALL_FILES=(
  "taskfiles/deps/installs/absl.yaml"
  "taskfiles/deps/installs/antlr-jar.yaml"
  "taskfiles/deps/installs/antlr-runtime.yaml"
  "taskfiles/deps/installs/boost.yaml"
  "taskfiles/deps/installs/catch2.yaml"
  "taskfiles/deps/installs/date.yaml"
  "taskfiles/deps/installs/fmt.yaml"
  "taskfiles/deps/installs/liblzma.yaml"
  "taskfiles/deps/installs/log-surgeon.yaml"
  "taskfiles/deps/installs/lz4.yaml"
  "taskfiles/deps/installs/mariadb-connector-cpp.yaml"
  "taskfiles/deps/installs/microsoft.gsl.yaml"
  "taskfiles/deps/installs/mongocxx.yaml"
  "taskfiles/deps/installs/msgpack-cxx.yaml"
  "taskfiles/deps/installs/nlohmann_json.yaml"
  "taskfiles/deps/installs/simdjson.yaml"
  "taskfiles/deps/installs/spdlog.yaml"
  "taskfiles/deps/installs/sqlite3.yaml"
  "taskfiles/deps/installs/utfcpp.yaml"
  "taskfiles/deps/installs/yaml-cpp.yaml"
  "taskfiles/deps/installs/ystdlib.yaml"
  "taskfiles/deps/installs/zlib.yaml"
  "taskfiles/deps/installs/zstd.yaml"
)

# Get the commit SHA from PR #1377
PR_1377_SHA=$(gh pr view 1377 --json commits --jq '.commits[-1].oid')
echo "PR #1377 latest commit: $PR_1377_SHA"

# Fetch the commit
git fetch origin "$PR_1377_SHA"

# Create a directory to store PR #1377 files
mkdir -p /tmp/pr_1377_files/taskfiles/deps/installs

# Extract files from the commit
for file in "${INSTALL_FILES[@]}"; do
  filename=$(basename "$file")
  echo "Extracting $file..."
  git show "$PR_1377_SHA:$file" > "/tmp/pr_1377_files/$file" 2>/dev/null && echo "  ✓ Extracted" || echo "  ✗ Not found"
done

echo ""
echo "=== Files extracted ==="
ls -la /tmp/pr_1377_files/taskfiles/deps/installs/

Length of output: 3315

🏁 Script executed:

#!/bin/bash

# Now let's check what files exist in the current branch
echo "=== Current branch install task files ==="
if [ -d "taskfiles/deps/installs" ]; then
  ls -la taskfiles/deps/installs/
else
  echo "Directory taskfiles/deps/installs does not exist in current branch"
fi

# Check if individual files exist
echo ""
echo "=== Checking individual install files in current branch ==="
INSTALL_FILES=(
  "taskfiles/deps/installs/absl.yaml"
  "taskfiles/deps/installs/antlr-jar.yaml"
  "taskfiles/deps/installs/antlr-runtime.yaml"
  "taskfiles/deps/installs/boost.yaml"
  "taskfiles/deps/installs/catch2.yaml"
  "taskfiles/deps/installs/date.yaml"
  "taskfiles/deps/installs/fmt.yaml"
  "taskfiles/deps/installs/liblzma.yaml"
  "taskfiles/deps/installs/log-surgeon.yaml"
  "taskfiles/deps/installs/lz4.yaml"
  "taskfiles/deps/installs/mariadb-connector-cpp.yaml"
  "taskfiles/deps/installs/microsoft.gsl.yaml"
  "taskfiles/deps/installs/mongocxx.yaml"
  "taskfiles/deps/installs/msgpack-cxx.yaml"
  "taskfiles/deps/installs/nlohmann_json.yaml"
  "taskfiles/deps/installs/simdjson.yaml"
  "taskfiles/deps/installs/spdlog.yaml"
  "taskfiles/deps/installs/sqlite3.yaml"
  "taskfiles/deps/installs/utfcpp.yaml"
  "taskfiles/deps/installs/yaml-cpp.yaml"
  "taskfiles/deps/installs/ystdlib.yaml"
  "taskfiles/deps/installs/zlib.yaml"
  "taskfiles/deps/installs/zstd.yaml"
)

for file in "${INSTALL_FILES[@]}"; do
  if [ -f "$file" ]; then
    echo "✓ $file exists"
  else
    echo "✗ $file does not exist"
  fi
done

Length of output: 1431

🏁 Script executed:

#!/bin/bash

# Let's look at the current main.yaml to see where the lib install tasks are defined
echo "=== Current taskfiles/deps/main.yaml content ==="
cat taskfiles/deps/main.yaml

echo ""
echo "=== Length of main.yaml ==="
wc -l taskfiles/deps/main.yaml

Length of output: 20928

🏁 Script executed:

#!/bin/bash

# Get main.yaml from PR #1377 to compare
PR_1377_SHA="a143bda8fd871a47d05af81b545319c0dc8fc2b9"

echo "=== PR #1377 main.yaml content ==="
git show "$PR_1377_SHA:taskfiles/deps/main.yaml" > /tmp/pr_1377_main.yaml
cat /tmp/pr_1377_main.yaml

echo ""
echo "=== Length of PR #1377 main.yaml ==="
wc -l /tmp/pr_1377_main.yaml

Length of output: 1386

🏁 Script executed:

#!/bin/bash

# Now let's analyze the current main.yaml to extract individual lib install tasks
# We'll need to identify each task block and its boundaries

echo "=== Extracting lib install task names from current main.yaml ==="
# Look for task definitions (lines starting with spaces followed by task name)
grep -E '^\s+[a-z0-9_-]+:$' taskfiles/deps/main.yaml | head -30

echo ""
echo "=== Sample of main.yaml structure ==="
head -100 taskfiles/deps/main.yaml

Length of output: 3492

🏁 Script executed:

#!/bin/bash

# Let's get a better understanding by looking at the structure
# Extract task names that match library names

PR_1377_SHA="a143bda8fd871a47d05af81b545319c0dc8fc2b9"

echo "=== Library install tasks in PR #1377 individual files ==="
ls -1 /tmp/pr_1377_files/taskfiles/deps/installs/*.yaml | while read file; do
  basename "$file" .yaml
done | sort

echo ""
echo "=== Searching for corresponding tasks in current main.yaml ==="
# The task names in main.yaml might be like "install-absl", "install-boost", etc.
grep -E '^\s+(install-|deps:install-)' taskfiles/deps/main.yaml | head -30

Length of output: 326

🏁 Script executed:

#!/bin/bash

# Let's extract each install task from current main.yaml
# First, let's understand the structure better

echo "=== Searching for install tasks in current main.yaml ==="
cat taskfiles/deps/main.yaml | grep -n "install-" | head -40

echo ""
echo "=== First 200 lines of current main.yaml ==="
head -200 taskfiles/deps/main.yaml

Length of output: 7819

🏁 Script executed:

#!/bin/bash

# Extract each library install task from current main.yaml and generate diffs
# We'll use Python to parse and extract each task properly

cat > /tmp/generate_diffs.py << 'PYTHON_SCRIPT'
import re
import subprocess

# Library names to extract
libs = [
    "absl", "antlr-jar", "antlr-runtime", "boost", "catch2", "date", "fmt",
    "liblzma", "log-surgeon", "lz4", "mariadb-connector-cpp", "microsoft.gsl",
    "mongocxx", "msgpack-cxx", "nlohmann_json", "simdjson", "spdlog", "sqlite3",
    "utfcpp", "yaml-cpp", "ystdlib", "zlib", "zstd"
]

# Read the current main.yaml
with open('taskfiles/deps/main.yaml', 'r') as f:
    main_yaml_lines = f.readlines()

# Function to extract a task from main.yaml
def extract_task(task_name, lines):
    """Extract a task block from the YAML lines."""
    task_lines = []
    in_task = False
    task_indent = None
    
    for i, line in enumerate(lines):
        # Check if this is the start of our task
        if re.match(r'^  ' + re.escape(task_name) + r':$', line):
            in_task = True
            task_indent = len(line) - len(line.lstrip())
            task_lines.append(line)
            continue
        
        if in_task:
            # Check if we've reached the next task at the same level
            if line.strip() and not line.startswith(' ' * (task_indent + 1)):
                # This is a new task at the same or higher level
                if re.match(r'^  [a-z0-9_-]+:$', line):
                    break
            task_lines.append(line)
    
    return ''.join(task_lines)

# Extract and create temp files for diffs
for lib in libs:
    current_task = extract_task(lib, main_yaml_lines)
    
    # Handle special cases with sub-tasks
    if lib == "liblzma":
        # Also extract liblzma-install task
        liblzma_install = extract_task("liblzma-install", main_yaml_lines)
        current_task += liblzma_install
    
    # Write current version to temp file
    current_file = f'/tmp/current_{lib}.yaml'
    with open(current_file, 'w') as f:
        f.write(current_task)
    
    # PR #1377 file path
    pr_1377_file = f'/tmp/pr_1377_files/taskfiles/deps/installs/{lib}.yaml'
    
    print(f"\n{'='*80}")
    print(f"DIFF: {lib}.yaml")
    print(f"Current (PR #1479) vs PR #1377")
    print(f"{'='*80}\n")
    
    # Generate diff
    result = subprocess.run(
        ['diff', '-u', pr_1377_file, current_file],
        capture_output=True,
        text=True
    )
    
    if result.returncode == 0:
        print(f"No differences found for {lib}.yaml")
    else:
        print(result.stdout)

PYTHON_SCRIPT

python /tmp/generate_diffs.py

Length of output: 24656

@Bill-hbrhbr Here are the diffs for each lib install task comparing the current version in PR #1479 against PR #1377:

Summary of Key Differences

The diffs reveal the structural changes between the two approaches:

PR #1377 (Modular Approach)

  • Each library has its own separate YAML file in taskfiles/deps/installs/
  • Each file has version: "3" declaration
  • Uses default: as the task name (standard for included taskfiles)
  • Task references use : prefix to reference parent namespace (e.g., :utils:install-remote-cmake-lib)
  • Some tasks use for loops for dependencies (log-surgeon, msgpack-cxx, spdlog, ystdlib)
  • Some use variables for versions (boost, mongocxx, msgpack-cxx)

Current PR #1479 (Monolithic Approach)

  • All library tasks embedded directly in taskfiles/deps/main.yaml
  • Task names match library names (e.g., absl:, boost:)
  • Task references are relative (e.g., utils:install-remote-cmake-lib)
  • Dependencies are listed explicitly rather than using for loops
  • Versions are hardcoded in URLs rather than using variables
  • Additional CMAKE_GEN_ARGS in ystdlib task (-DCMAKE_BUILD_TYPE=Release, -DCMAKE_INSTALL_MESSAGE=LAZY)

Notable Content Differences

  1. ystdlib: Current version includes additional CMAKE arguments
  2. liblzma: Slightly different YAML formatting for TARBALL_URL (folded string vs. literal)
  3. boost, mongocxx, msgpack-cxx: PR refactor(taskfile): Split main dependency Taskfile into category-specific modules; Migrate antlr-runtime and utfcpp to use utils:install-remote-cmake-lib. #1377 uses version variables, current has hardcoded versions

All 23 library install task diffs are shown in the output above. The main difference is the organizational structure, with PR #1377 splitting each library into its own file following a standard pattern, while the current PR keeps them all in main.yaml.

@Bill-hbrhbr Bill-hbrhbr changed the title chore(deps): Define standard rules for writing lib install tasks and apply across all existing tasks. chore(deps): Document standard rules for writing lib install tasks and apply across all existing tasks. Oct 23, 2025
@Bill-hbrhbr Bill-hbrhbr changed the title chore(deps): Document standard rules for writing lib install tasks and apply across all existing tasks. refactor(deps): Document standard rules for writing lib install tasks and apply across all existing tasks. Oct 23, 2025
@Bill-hbrhbr Bill-hbrhbr changed the title refactor(deps): Document standard rules for writing lib install tasks and apply across all existing tasks. refactor(deps): Document standard rules for writing lib install tasks and apply across all existing tasks: Oct 23, 2025
@Bill-hbrhbr Bill-hbrhbr marked this pull request as ready for review October 23, 2025 16:31
@Bill-hbrhbr Bill-hbrhbr requested a review from a team as a code owner October 23, 2025 16:31
coderabbitai[bot]
coderabbitai bot reviewed Oct 23, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ff731b and 90bbe66.

📒 Files selected for processing (2)
  • taskfiles/deps/README.md (1 hunks)
  • taskfiles/deps/main.yaml (14 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings 
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-05-28T18:33:30.155Z 
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.

Applied to files:

  • taskfiles/deps/main.yaml
📚 Learning: 2025-09-15T18:53:15.844Z 
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1226
File: taskfiles/deps/main.yaml:522-527
Timestamp: 2025-09-15T18:53:15.844Z
Learning: When ystdlib is built with -Dystdlib_BUILD_TESTING=OFF in taskfiles/deps/main.yaml, it does not require Catch2 settings to be injected via CMAKE_GEN_ARGS since testing is disabled and Catch2 is not needed during the build process.

Applied to files:

  • taskfiles/deps/main.yaml
📚 Learning: 2025-08-19T07:08:15.583Z 
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1229
File: taskfiles/deps/main.yaml:494-504
Timestamp: 2025-08-19T07:08:15.583Z
Learning: In zlib v1.3.1's CMake build system, only ZLIB_BUILD_EXAMPLES is a valid option. Other ZLIB_BUILD_* flags (MINIZIP, SHARED, STATIC, TESTING) and ZLIB_INSTALL are not defined in the CMakeLists.txt and have no effect. Use BUILD_SHARED_LIBS=ON for shared library control and ZLIB_BUILD_EXAMPLES=OFF to disable examples.

Applied to files:

  • taskfiles/deps/main.yaml
🪛 LanguageTool
taskfiles/deps/README.md

[uncategorized] ~14-~14: Possible missing comma found.
Context: ...d parsing version numbers from download URLs unless the version is used elsewhere. ...

(AI_HYDRA_LEO_MISSING_COMMA)

[style] ~30-~30: Consider using a different verb to strengthen your wording.
Context: ...s, examples, docs or unused binaries to speed up the install process. - Prefer disablin...

(SPEED_UP_ACCELERATE)

[grammar] ~30-~30: The word ‘install’ is not a noun.
Context: ...docs or unused binaries to speed up the install process. - Prefer disabling lib-specif...

(A_INSTALL)

🪛 markdownlint-cli2 (0.18.1)
taskfiles/deps/README.md

37-37: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

38-38: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

🔇 Additional comments (6)
taskfiles/deps/main.yaml (6)

155-186: LGTM: antlr-jar and antlr-runtime refactoring is clean.

The antlr-jar task correctly references the global G_ANTLR_JAR_FILE variable, and the antlr-runtime task is properly refactored to use install-remote-cmake-lib with appropriate CMake flags. The CMP0135 policy and comment explaining its purpose are well preserved.

296-313: Verify whether log-surgeon needs CMP0074=NEW policy.

The task uses -C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_FMT_LIB_NAME}}.cmake and {{.G_GSL_LIB_NAME}}.cmake config files for dependencies, which may internally use <PackageName>_ROOT variables. Per the README, CMP0074 should be set to NEW when the component depends on another via <lib_name>_ROOT. Compare this to msgpack-cxx (line 401) and spdlog (line 454), which both set CMAKE_POLICY_DEFAULT_CMP0074=NEW when using similar config files.

Does log-surgeon's CMakeLists.txt use find_package(fmt) and find_package(Microsoft.GSL) that would benefit from CMP0074? If so, add:

  CMAKE_GEN_ARGS:
    - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_FMT_LIB_NAME}}.cmake"
    - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_GSL_LIB_NAME}}.cmake"
+   - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW"
    - "-DCMAKE_BUILD_TYPE=Release"

530-546: Verify whether ystdlib needs CMP0074=NEW policy.

The task uses -C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake to load Boost configuration, which is the same pattern as msgpack-cxx (line 401) and spdlog (line 454). Both of those tasks explicitly set CMAKE_POLICY_DEFAULT_CMP0074=NEW, but ystdlib does not.

Confirm whether ystdlib's CMakeLists.txt uses find_package(Boost) in a way that would benefit from CMP0074. If so, add it to CMAKE_GEN_ARGS:

  CMAKE_GEN_ARGS:
    - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake"
+   - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW"
    - "-DCMAKE_BUILD_TYPE=Release"

465-514: LGTM: sqlite3 and utfcpp follow new naming conventions.

Both tasks correctly use the -extracted directory suffix as per README guidance. The direct CMake config file generation via heredoc is appropriate for these special cases that don't use standard CMake install paths.

548-564: LGTM: zlib CMake arguments are correct.

Uses only valid zlib v1.3.1 configuration options (specifically ZLIB_BUILD_EXAMPLES=OFF rather than invalid ZLIB_BUILD_* variants). The special CMAKE_INSTALL_PREFIX handling is appropriate for zlib's structure.

241-256: Verify whether fmt should build both shared and static libraries.

Per the README, tasks should "build both shared and static libraries when possible." The fmt task (line 248) only sets BUILD_SHARED_LIBS=ON. Verify whether fmt's CMake supports static library builds and whether both variants are needed by downstream consumers.

If fmt supports and should build static libraries, add a corresponding flag or consider a pattern similar to liblzma (which invokes the task twice with different BUILD_SHARED_LIBS values).

sitaowang1998
sitaowang1998 requested changes Oct 23, 2025
View reviewed changes
Copy link
Contributor

@sitaowang1998 sitaowang1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code lgtm. The doc needs some more works.

sitaowang1998
sitaowang1998 previously approved these changes Oct 27, 2025
View reviewed changes
Copy link
Contributor

@sitaowang1998 sitaowang1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This PR just makes me think that we need some documents in yscope-dev-utils to explain how to use the tasks. Some logic, e.g. checksum validation should be in task dependency, is hard to understand at first glance. Do you think it worth the effort @kirkrodrigues ?

@Bill-hbrhbr
Copy link
Contributor Author

Bill-hbrhbr commented Oct 28, 2025 •
edited
Loading

LGTM. This PR just makes me think that we need some documents in yscope-dev-utils to explain how to use the tasks. Some logic, e.g. checksum validation should be in task dependency, is hard to understand at first glance. Do you think it worth the effort @kirkrodrigues ?

@davidlion In the near future, we could add some simple docs on how-to-use the checksum stuff.

@Bill-hbrhbr Bill-hbrhbr requested a review from davidlion October 30, 2025 17:16
@LinZhihao-723 LinZhihao-723 self-requested a review November 5, 2025 22:42
LinZhihao-723
LinZhihao-723 requested changes Nov 5, 2025
View reviewed changes
taskfiles/deps/main.yaml
echo "set(CLP_SQLITE3_INCLUDE_DIRECTORY \"{{.INSTALL_INCLUDE_DIR}}\")" \
>> "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake"
mkdir -p "{{.INSTALL_INCLUDE_DIR}}"
ln -sfn "{{.SRC_DIR}}" "{{.INSTALL_SYMLINK}}"
Copy link
Member

@LinZhihao-723 LinZhihao-723 Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think we prefer to copy instead of using symlink.
  • If we want to use symlink, can u expend -sfn with the long forms like --symbolic --force --no-derefernece? The guideline requires the use of the full term for clarity unless it has a macOS compatibility issue.

Copy link
Contributor Author

@Bill-hbrhbr Bill-hbrhbr Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ln doesn't have long options on macos. Ref: https://ss64.com/mac/ln.html

coderabbitai[bot]
coderabbitai bot reviewed Nov 7, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
taskfiles/deps/README.md (1)

9-14: Consider adding explicit guidance on yscope-dev-utils:checksum task usage.

While lines 12–13 mention that checksum validation is encouraged through yscope-dev-utils tasks, the documentation could be clearer by explicitly referencing the yscope-dev-utils:checksum task and its usage pattern. This would give contributors a concrete reference when implementing checksum validation in new tasks.

If you'd like, I can help draft an expanded section with specific usage examples for checksum validation. Would that be helpful?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad9e783 and a59858d.

📒 Files selected for processing (1)
  • taskfiles/deps/README.md (1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

Learnt from: haiqi96
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.

Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
📚 Learning: 2025-05-28T18:33:30.155Z 
Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-04T18:38:33.130Z 
Learnt from: haiqi96
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-23T09:54:45.185Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-09T04:07:27.083Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-01T14:52:15.217Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-01T14:52:15.217Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-07T17:41:15.655Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-03-18T07:27:54.738Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-17T16:19:24.478Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/README.md:5-6
Timestamp: 2025-08-17T16:19:24.478Z
Learning: In the CLP project, README.md files can have multiple H1 headings when it makes sense structurally (e.g., "Contributing" as a top-level section alongside the main title). The MD025 markdown linting rule should be ignored in such cases where multiple H1s are intentional.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-05T16:13:24.482Z 
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1154
File: docs/src/user-guide/guides-using-presto.md:53-55
Timestamp: 2025-08-05T16:13:24.482Z
Learning: In the CLP project's documentation system, numbered list items with line breaks do not require additional indentation for continuation lines to remain part of the list item - the markdown parser correctly keeps wrapped content within the numbered list item even without strict indentation.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-05-06T09:48:55.408Z 
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-10-17T19:59:35.299Z 
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:757-757
Timestamp: 2025-10-17T19:59:35.299Z
Learning: In the y-scope/clp repository, prefer long-form command-line flags (e.g., `--recursive`) over short-form flags (e.g., `-R`) for readability, even if the long form is GNU-specific.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-29T07:26:53.532Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:27-33
Timestamp: 2025-08-29T07:26:53.532Z
Learning: In CLP's build tool installation scripts, CMake version constraints should accommodate platform differences rather than using exact version pinning. Range constraints like "cmake>=3.23,<3.24" are preferred over exact pinning (cmake==3.23.5) to allow for platform-specific package availability while maintaining required version bounds.

Applied to files:

  • taskfiles/deps/README.md
🪛 LanguageTool
taskfiles/deps/README.md

[style] ~32-~32: Consider using a different verb to strengthen your wording.
Context: ...s, examples, docs or unused binaries to speed up the installation process. * Prefer dis...

(SPEED_UP_ACCELERATE)

@Bill-hbrhbr @coderabbitai
Update taskfiles/deps/README.md
e75c8de 
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
coderabbitai[bot]
coderabbitai bot reviewed Nov 7, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a59858d and e75c8de.

📒 Files selected for processing (1)
  • taskfiles/deps/README.md (1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

Learnt from: haiqi96
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.

Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.

Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
📚 Learning: 2025-05-28T18:33:30.155Z 
Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-23T09:54:45.185Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-04T18:38:33.130Z 
Learnt from: haiqi96
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-01T14:52:15.217Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-01T14:52:15.217Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-07T17:41:15.655Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-09T04:07:27.083Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-03-18T07:27:54.738Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-17T16:19:24.478Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/README.md:5-6
Timestamp: 2025-08-17T16:19:24.478Z
Learning: In the CLP project, README.md files can have multiple H1 headings when it makes sense structurally (e.g., "Contributing" as a top-level section alongside the main title). The MD025 markdown linting rule should be ignored in such cases where multiple H1s are intentional.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-05T16:13:24.482Z 
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1154
File: docs/src/user-guide/guides-using-presto.md:53-55
Timestamp: 2025-08-05T16:13:24.482Z
Learning: In the CLP project's documentation system, numbered list items with line breaks do not require additional indentation for continuation lines to remain part of the list item - the markdown parser correctly keeps wrapped content within the numbered list item even without strict indentation.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-05-06T09:48:55.408Z 
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-10-17T19:59:35.299Z 
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:757-757
Timestamp: 2025-10-17T19:59:35.299Z
Learning: In the y-scope/clp repository, prefer long-form command-line flags (e.g., `--recursive`) over short-form flags (e.g., `-R`) for readability, even if the long form is GNU-specific.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-29T07:26:53.532Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:27-33
Timestamp: 2025-08-29T07:26:53.532Z
Learning: In CLP's build tool installation scripts, CMake version constraints should accommodate platform differences rather than using exact version pinning. Range constraints like "cmake>=3.23,<3.24" are preferred over exact pinning (cmake==3.23.5) to allow for platform-specific package availability while maintaining required version bounds.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-06-02T18:22:24.060Z 
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.

Applied to files:

  • taskfiles/deps/README.md
🪛 LanguageTool
taskfiles/deps/README.md

[style] ~32-~32: Consider using a different verb to strengthen your wording.
Context: ...s, examples, docs or unused binaries to speed up the installation process. * Prefer dis...

(SPEED_UP_ACCELERATE)

🔇 Additional comments (2)
taskfiles/deps/README.md (2)

5-7: Clear preference hierarchy is well-established.

The documentation effectively communicates that deps:utils:install-remote-cmake-lib is preferred through the "descending order of preference" statement, which addresses previous feedback.

12-13: Checksum validation guidance aligns with PR objectives.

The section appropriately documents best-effort checksum validation and encourages use of yscope-dev-utils tasks for proper checks, addressing Bill-hbrhbr's suggestion to document this practice for future clarity.

coderabbitai[bot]
coderabbitai bot reviewed Nov 10, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ef92df and 5952c08.

📒 Files selected for processing (3)
  • components/core/CMakeLists.txt (2 hunks)
  • taskfiles/deps/README.md (1 hunks)
  • taskfiles/deps/main.yaml (14 hunks)
🧰 Additional context used
🧠 Learnings (28)
📓 Common learnings 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

Learnt from: haiqi96
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.

Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.

Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
📚 Learning: 2025-08-09T04:07:27.083Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/README.md
  • taskfiles/deps/main.yaml
📚 Learning: 2025-07-23T09:54:45.185Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/README.md
  • taskfiles/deps/main.yaml
📚 Learning: 2025-08-13T19:58:26.058Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/main.yaml
📚 Learning: 2025-06-02T18:22:24.060Z 
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/README.md
  • taskfiles/deps/main.yaml
📚 Learning: 2025-08-03T18:56:07.366Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/main.yaml
📚 Learning: 2025-08-06T07:32:28.462Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/main.yaml
📚 Learning: 2025-08-19T07:08:15.583Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1229
File: taskfiles/deps/main.yaml:494-504
Timestamp: 2025-08-19T07:08:15.583Z
Learning: In zlib v1.3.1's CMake build system, only ZLIB_BUILD_EXAMPLES is a valid option. Other ZLIB_BUILD_* flags (MINIZIP, SHARED, STATIC, TESTING) and ZLIB_INSTALL are not defined in the CMakeLists.txt and have no effect. Use BUILD_SHARED_LIBS=ON for shared library control and ZLIB_BUILD_EXAMPLES=OFF to disable examples.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/main.yaml
📚 Learning: 2025-07-24T21:56:05.806Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.

Applied to files:

  • components/core/CMakeLists.txt
📚 Learning: 2025-10-14T20:35:29.944Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1377
File: taskfiles/deps/cmake-libs.yaml:240-260
Timestamp: 2025-10-14T20:35:29.944Z
Learning: In msgpack-cxx, the CMake option `MSGPACK_BUILD_TESTS` defaults to `OFF`, so it does not need to be explicitly disabled in CMAKE_GEN_ARGS.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/main.yaml
📚 Learning: 2025-09-03T07:29:35.223Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.

Applied to files:

  • components/core/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z 
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.

Applied to files:

  • components/core/CMakeLists.txt
📚 Learning: 2025-08-04T17:26:17.098Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/README.md
📚 Learning: 2025-08-25T07:27:19.449Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1262
File: docs/src/dev-guide/components-core/index.md:50-50
Timestamp: 2025-08-25T07:27:19.449Z
Learning: In the CLP project, nlohmann_json version 3.11.3 is installed through their dependency management process, but they don't enforce version constraints in CMake find_package calls - they control versions through installation rather than CMake version pinning.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/main.yaml
📚 Learning: 2025-08-18T05:43:07.868Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.

Applied to files:

  • components/core/CMakeLists.txt
📚 Learning: 2025-08-06T08:10:26.381Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:195-202
Timestamp: 2025-08-06T08:10:26.381Z
Learning: According to Bill-hbrhbr, in LZ4's CMake configuration, the correct package name for find_package is lowercase "lz4", and the exported targets use uppercase namespace "LZ4::" with lowercase prefixes (LZ4::lz4_static, LZ4::lz4_shared), not lowercase namespace "lz4::" as initially assumed.

Applied to files:

  • components/core/CMakeLists.txt
  • taskfiles/deps/main.yaml
📚 Learning: 2025-05-28T18:33:30.155Z 
Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.

Applied to files:

  • taskfiles/deps/README.md
  • taskfiles/deps/main.yaml
📚 Learning: 2025-07-01T14:52:15.217Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-01T14:52:15.217Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-07-07T17:41:15.655Z 
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Applied to files:

  • taskfiles/deps/README.md
  • taskfiles/deps/main.yaml
📚 Learning: 2025-03-18T07:27:54.738Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-17T16:19:24.478Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/README.md:5-6
Timestamp: 2025-08-17T16:19:24.478Z
Learning: In the CLP project, README.md files can have multiple H1 headings when it makes sense structurally (e.g., "Contributing" as a top-level section alongside the main title). The MD025 markdown linting rule should be ignored in such cases where multiple H1s are intentional.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-08-05T16:13:24.482Z 
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1154
File: docs/src/user-guide/guides-using-presto.md:53-55
Timestamp: 2025-08-05T16:13:24.482Z
Learning: In the CLP project's documentation system, numbered list items with line breaks do not require additional indentation for continuation lines to remain part of the list item - the markdown parser correctly keeps wrapped content within the numbered list item even without strict indentation.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-05-06T09:48:55.408Z 
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-10-17T19:59:35.299Z 
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:757-757
Timestamp: 2025-10-17T19:59:35.299Z
Learning: In the y-scope/clp repository, prefer long-form command-line flags (e.g., `--recursive`) over short-form flags (e.g., `-R`) for readability, even if the long form is GNU-specific.

Applied to files:

  • taskfiles/deps/README.md
  • taskfiles/deps/main.yaml
📚 Learning: 2025-08-29T07:26:53.532Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:27-33
Timestamp: 2025-08-29T07:26:53.532Z
Learning: In CLP's build tool installation scripts, CMake version constraints should accommodate platform differences rather than using exact version pinning. Range constraints like "cmake>=3.23,<3.24" are preferred over exact pinning (cmake==3.23.5) to allow for platform-specific package availability while maintaining required version bounds.

Applied to files:

  • taskfiles/deps/README.md
📚 Learning: 2025-09-15T18:53:15.844Z 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1226
File: taskfiles/deps/main.yaml:522-527
Timestamp: 2025-09-15T18:53:15.844Z
Learning: When ystdlib is built with -Dystdlib_BUILD_TESTING=OFF in taskfiles/deps/main.yaml, it does not require Catch2 settings to be injected via CMAKE_GEN_ARGS since testing is disabled and Catch2 is not needed during the build process.

Applied to files:

  • taskfiles/deps/README.md
  • taskfiles/deps/main.yaml
📚 Learning: 2024-10-22T15:36:04.655Z 
Learnt from: gibber9809
Repo: y-scope/clp PR: 504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.

Applied to files:

  • taskfiles/deps/main.yaml
🪛 LanguageTool
taskfiles/deps/README.md

[uncategorized] ~10-~10: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...f the library can be installed via CMake but a custom approach is used, briefly docu...

(COMMA_COMPOUND_SENTENCE_2)

[style] ~39-~39: Consider using a different verb to strengthen your wording.
Context: ..., examples, docs, or unused binaries to speed up the installation process. * Prefer dis...

(SPEED_UP_ACCELERATE)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-macos (macos-14, false)
  • GitHub Check: rust-checks (ubuntu-22.04)
  • GitHub Check: build (macos-15)
🔇 Additional comments (21)
components/core/CMakeLists.txt (3)

300-301: TODO comment documents planned refactor; aligns with learnings.

The inline TODO appropriately defers global replacement of zstd_TARGET references to a separate PR, consistent with team practice for focused migration efforts. This is aligned with the learning from PR #1156 that such refactoring improvements should be tracked separately.

304-308: LibLZMA discovery simplified; verify version requirement is sufficient.

The custom Find module logic has been removed in favour of a simple find_package(LibLZMA 5.8.1 REQUIRED) call. Line 305 references version 5.8.1 to address CVE-2024-3094 and CVE-2025-31115. Confirm that this centralized approach (removing FindLibLZMA.cmake) works correctly with the centralized installer configuration in taskfiles/deps/main.yaml and that the version constraint is enforced throughout the build pipeline.

Based on learnings, the custom Find module supported flexible static/shared switching. Verify that relying solely on centralized installation and find_package() does not introduce version mismatches or incorrect library variant selection in downstream consumers.

272-272: Hardcoded static target violates established build pattern and will fail in non-static builds.

The assignment unconditionally uses mongo::mongocxx_static, but all other libraries in the same file (Boost, libarchive, MariaDBClient) conditionally select targets based on CLP_USE_STATIC_LIBS. This inconsistency will cause linking failures when CLP_USE_STATIC_LIBS is OFF and only the shared mongocxx variant is available. The mongocxx target selection must follow the same conditional pattern:

if(CLP_USE_STATIC_LIBS)
    set(MONGOCXX_TARGET mongo::mongocxx_static)
else()
    set(MONGOCXX_TARGET mongo::mongocxx_shared)
endif()

Alternatively, if static-only is intentional, document why mongocxx differs from all other dependencies.

⛔ Skipped due to learnings 
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.

Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.

Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
taskfiles/deps/README.md (1)

44-47: CMake policy and exception rules are well-documented.

The guidance on CMAKE_BUILD_TYPE and CMAKE_INSTALL_MESSAGE is clear and provides specific justification for these exceptions. This will help future maintainers understand when to override the "remove redundant flags" principle.

taskfiles/deps/main.yaml (17)

94-94: Platform check correctly uses POSIX-compatible dot-sourcing syntax.

Line 94 updates the Ubuntu platform check to use . /etc/os-release && [ "$ID" = "ubuntu" ], which is POSIX-compatible (dot-sourcing without source keyword). This aligns with the PR objective to make platform checks POSIX-compatible and matches the pattern applied to the spider task's precondition. Confirmed via learnings that explicit source was avoided for POSIX compatibility.

160-160: antlr-jar task simplified correctly.

The output file reference is now derived from the global variable G_ANTLR_JAR_FILE instead of being exposed as a public variable. This reduces the task's surface area and aligns with the consolidation goal. Removal of CHECKSUM_FILE and manual checksum logic in favour of the centralized yscope-dev-utils:remote:curl task simplifies maintenance.

167-183: antlr-runtime migrated to centralized installer with correct CMAKE arguments.

The task now uses utils:install-remote-cmake-lib instead of custom build logic. Verified correct CMAKE_GEN_ARGS:

  • ANTLR_BUILD_CPP_TESTS=OFF and ANTLR_BUILD_SHARED=OFF: appropriate for building static-only libraries.
  • CMAKE_POLICY_DEFAULT_CMP0135=NEW: ensures extracted files use current timestamp, necessary for proper rebuild detection.
  • CMAKE_SOURCE_DIR: "runtime/Cpp": correctly points to the C++ runtime subdirectory within the antlr4 archive.
  • Checksum and URL are hardcoded for maintainability (avoids version parsing from URLs per documentation).

Verify that the SHA256 9f18272a9b32b622835a3365f850dd1063d60f5045fb1e12ce475ae6e18a35bb for antlr4 v4.13.2 archive is correct.

188-189: Boost task now includes explicit deps section for clarity.

Adding the deps: [utils:init] section makes the task's dependencies explicit even though boost uses a custom installer task rather than the centralized pattern. This improves readability and ensures consistent initialization.

214-216: Catch2 CMAKE_GEN_ARGS correctly disables tests, docs, and extras using lib-specific flags.

The flags CATCH_BUILD_TESTING, CATCH_INSTALL_DOCS, and CATCH_INSTALL_EXTRAS are all lib-specific (preferred over generic BUILD_TESTING per guidelines). These appropriately reduce the installation footprint.

260-273: LibLZMA refactored with comprehensive CMAKE_GEN_ARGS for consistency.

The task now uses centralized install-remote-cmake-lib with extensive CMAKE_GEN_ARGS:

  • BUILD_TESTING=OFF, XZ_DOC=OFF: disable unnecessary components.
  • XZ_TOOL_*=OFF: disable tools (xz, xzdec, lzmainfo, lzmadec, scripts, symlinks) not needed by CLP.
  • CMAKE_BUILD_TYPE=Release and CMAKE_INSTALL_MESSAGE=LAZY: follow documented conventions.
  • Version constraint 5.8.1 addresses CVE-2024-3094 and CVE-2025-31115 (documented in CMakeLists.txt line 305).

This consolidation removes the custom FindLibLZMA.cmake logic and aligns with the centralized approach seen throughout the PR. TARBALL_URL and TARBALL_SHA256 are hardcoded for stability.

Verify SHA256 507825b599356c10dca1cd720c9d0d0c9d5400b9de300af00e4d1ea150795543 for xz v5.8.1 release archive.

301-301: LZ4 CMAKE_GEN_ARGS correctly uses BUILD_SHARED_LIBS=OFF to build static-only.

Per learnings from PR #1156, LZ4's CMake build system uses BUILD_SHARED_LIBS (generic CMake convention) not LZ4-specific flags for library variant control. Setting BUILD_SHARED_LIBS=OFF is correct for static-only building.

319-319: MariaDB-connector-cpp precondition updated to POSIX-compatible dot-sourcing.

Mirrors the Ubuntu platform check update in the spider task (line 94), using . /etc/os-release && [ "$ID" = "ubuntu" ] for POSIX compatibility.

326-331: MariaDB-connector-cpp CMAKE_GEN_ARGS correctly configured for DEB layout.

The task sets INSTALL_LAYOUT=DEB and USE_SYSTEM_INSTALLED_LIB=ON as required, disables tests with WITH_UNIT_TESTS=OFF, and follows the documented conventions. The hardcoded version (1.1.5) with SHA256 ensures reproducibility.

359-359: Mongocxx adds explicit ENABLE_EXAMPLES=OFF flag.

The task correctly adds the flag to disable examples during installation, reducing the footprint. This is a lib-specific flag following documented best practices.

363-364: TARBALL_URL formatting with line continuation is consistent and correct.

Both mongocxx and msgpack-cxx TARBALL_URLs use YAML multi-line string formatting with proper indentation for readability. URLs are hardcoded and version-pinned in the URL itself (mongocxx r4.1.1, msgpack-cxx 7.0.0), avoiding version parsing.

Also applies to: 383-384

445-445: SQLite3 extraction directory renamed to follow -extracted naming convention.

The variable EXTRACTION_DIR is now {{.G_DEPS_CPP_DIR}}/{{.LIB_NAME}}-extracted, consistent with the documented guideline and applied uniformly across all library tasks in this refactoring (e.g., liblzma, utfcpp).

462-466: SQLite3 cmake settings file generation is correct.

The generated cmake settings file properly sets CLP_SQLITE3_SOURCE_DIRECTORY and CLP_SQLITE3_INCLUDE_DIRECTORY using heredoc syntax, matching the pattern used in utfcpp (lines 485-488). This exposes the paths for use by the main CMakeLists.txt.

472-472: UTF-CPP output directory renamed and cmake settings generation simplified.

The variable UTFCPP_OUTPUT_DIR is renamed to use the -extracted suffix (line 472), consistent with newly documented convention. The cmake settings generation (lines 485-488) is streamlined to a single variable CLP_UTFCPP_INCLUDE_DIRECTORY pointing to the extracted directory. The comment explains this library does not use CMake installation but rather direct source inclusion, aligning with documented special case handling.

Also applies to: 485-488

499-499: YAML-CPP uses lib-specific test flag instead of generic BUILD_TESTING.

Line 499 correctly uses YAML_CPP_BUILD_TESTS=OFF (lib-specific flag per documentation) instead of the generic BUILD_TESTING=OFF. This is consistent with the documented guideline to prefer lib-specific testing flags.

514-515: ystdlib now includes CMAKE_BUILD_TYPE=Release and CMAKE_INSTALL_MESSAGE=LAZY.

These additions align with the documented exceptions to the "remove redundant flags" rule. Even if ystdlib's CMakeLists.txt has defaults, explicitly setting these here ensures consistent Release builds and reduced log verbosity across the dependency installation pipeline.

549-549: zstd explicitly disables shared library building.

Line 549 adds ZSTD_BUILD_SHARED=OFF to ensure static-only building, consistent with the static-library focus documented in the README and applied across other libraries (lz4, antlr-runtime, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@sitaowang1998 sitaowang1998 sitaowang1998 left review comments

@junhaoliao junhaoliao Awaiting requested review from junhaoliao

@davidlion davidlion Awaiting requested review from davidlion

@LinZhihao-723 LinZhihao-723 Awaiting requested review from LinZhihao-723

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Bill-hbrhbr @sitaowang1998 @LinZhihao-723 @davidlion